Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

deps: V8: backport f7771e5b0cc4 #31957

Closed
wants to merge 1 commit into from
Closed

deps: V8: backport f7771e5b0cc4 #31957

wants to merge 1 commit into from

Conversation

mmarchini
Copy link
Contributor

This fixes a performance regression on V8 where frequent delete/reinsertions on a POJO object can lead to a bit overflow, causing subsequent property insertions to take exponentially more time.

We should try to backport this to v12, since the regression started there (yes, I'm volunteering to backport if it doesn't land cleanly).

Original commit message:

[runtime] Recompute enumeration indices of dictionaries upon bitfield overflow

Otherwise we'll get weird semantics when enumerating objects after many
deletes/reinserts.

Bug: chromium:1033771
Change-Id: If0a459169c3794a30d9632d09e80da3cfcd4302c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1993966
Commit-Queue: Toon Verwaest <[email protected]>
Reviewed-by: Jakob Kummerow <[email protected]>
Reviewed-by: Victor Gomes <[email protected]>
Cr-Commit-Position: refs/heads/master@{#65690}

Refs: v8/v8@f7771e5

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@mmarchini
deps: V8: backport f7771e5b0cc4
d47ad3e 
Original commit message:

    [runtime] Recompute enumeration indices of dictionaries upon bitfield overflow

    Otherwise we'll get weird semantics when enumerating objects after many
    deletes/reinserts.

    Bug: chromium:1033771
    Change-Id: If0a459169c3794a30d9632d09e80da3cfcd4302c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1993966
    Commit-Queue: Toon Verwaest <[email protected]>
    Reviewed-by: Jakob Kummerow <[email protected]>
    Reviewed-by: Victor Gomes <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#65690}

Refs: v8/v8@f7771e5
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Feb 26, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 26, 2020 •
edited by mmarchini
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/29357/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/2891/

rerun linux: https://ci.nodejs.org/job/node-test-commit-linux/33112/
rerun linux, take 2: https://ci.nodejs.org/job/node-test-commit-linux/33114/

@addaleax addaleax mentioned this pull request Feb 26, 2020
Closed
@mmarchini mmarchini added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 26, 2020
MylesBorins
MylesBorins approved these changes Feb 28, 2020
View reviewed changes
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

V8 CI is failing on two platforms but those are known failures. Re ran linux CI which was having an issue but this should be ready to land

@MylesBorins
Copy link
Contributor

BTW this patch lands cleanly on both master and 13.x but will require a manual backport to 12.x

mmarchini added a commit that referenced this pull request Feb 28, 2020
@mmarchini
deps: V8: backport f7771e5b0cc4
3d894d0 
Original commit message:

    [runtime] Recompute enumeration indices of dictionaries upon bitfield overflow

    Otherwise we'll get weird semantics when enumerating objects after many
    deletes/reinserts.

    Bug: chromium:1033771
    Change-Id: If0a459169c3794a30d9632d09e80da3cfcd4302c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1993966
    Commit-Queue: Toon Verwaest <[email protected]>
    Reviewed-by: Jakob Kummerow <[email protected]>
    Reviewed-by: Victor Gomes <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#65690}

Refs: v8/v8@f7771e5

PR-URL: #31957
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@mmarchini
Copy link
Contributor Author

Landed in 3d894d0

@mmarchini mmarchini closed this Feb 28, 2020
@codebytere
Copy link
Member

@MylesBorins i just tried and it seems to conflict on v13 🤔

node on git:v13.x-staging ❯ g cp 3d894d0511                                3:09PM
error: could not apply 3d894d0511... deps: V8: backport f7771e5b0cc4
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'
Recorded preimage for 'common.gypi'

Not sure what the embedder string there should be, that's the conflict source.

@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 1, 2020 via email

codebytere pushed a commit that referenced this pull request Mar 1, 2020
@mmarchini @codebytere
deps: V8: backport f7771e5b0cc4
e6125cd 
Original commit message:

    [runtime] Recompute enumeration indices of dictionaries upon bitfield overflow

    Otherwise we'll get weird semantics when enumerating objects after many
    deletes/reinserts.

    Bug: chromium:1033771
    Change-Id: If0a459169c3794a30d9632d09e80da3cfcd4302c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1993966
    Commit-Queue: Toon Verwaest <[email protected]>
    Reviewed-by: Jakob Kummerow <[email protected]>
    Reviewed-by: Victor Gomes <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#65690}

Refs: v8/v8@f7771e5

PR-URL: #31957
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@codebytere codebytere mentioned this pull request Mar 1, 2020
Merged
mmarchini added a commit to mmarchini/node that referenced this pull request Mar 2, 2020
@mmarchini
deps: V8: backport f7771e5b0cc4
63049da 
Original commit message:

    [runtime] Recompute enumeration indices of dictionaries upon bitfield overflow

    Otherwise we'll get weird semantics when enumerating objects after many
    deletes/reinserts.

    Bug: chromium:1033771
    Change-Id: If0a459169c3794a30d9632d09e80da3cfcd4302c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1993966
    Commit-Queue: Toon Verwaest <[email protected]>
    Reviewed-by: Jakob Kummerow <[email protected]>
    Reviewed-by: Victor Gomes <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#65690}

Refs: v8/v8@f7771e5

PR-URL: nodejs#31957
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
This was referenced Mar 2, 2020
Closed
Closed
@ex1st ex1st mentioned this pull request Mar 10, 2020
Closed
codebytere pushed a commit that referenced this pull request Mar 17, 2020
@mmarchini @codebytere
deps: V8: backport f7771e5b0cc4
53305de 
Original commit message:

    [runtime] Recompute enumeration indices of dictionaries upon bitfield overflow

    Otherwise we'll get weird semantics when enumerating objects after many
    deletes/reinserts.

    Bug: chromium:1033771
    Change-Id: If0a459169c3794a30d9632d09e80da3cfcd4302c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1993966
    Commit-Queue: Toon Verwaest <[email protected]>
    Reviewed-by: Jakob Kummerow <[email protected]>
    Reviewed-by: Victor Gomes <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#65690}

Refs: v8/v8@f7771e5

PR-URL: #31957
Backport-PR-URL: #32053
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@codebytere codebytere mentioned this pull request Mar 17, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 23, 2020
@mmarchini @codebytere
deps: V8: backport f7771e5b0cc4
e6d08f9 
Original commit message:

    [runtime] Recompute enumeration indices of dictionaries upon bitfield overflow

    Otherwise we'll get weird semantics when enumerating objects after many
    deletes/reinserts.

    Bug: chromium:1033771
    Change-Id: If0a459169c3794a30d9632d09e80da3cfcd4302c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1993966
    Commit-Queue: Toon Verwaest <[email protected]>
    Reviewed-by: Jakob Kummerow <[email protected]>
    Reviewed-by: Victor Gomes <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#65690}

Refs: v8/v8@f7771e5

PR-URL: #31957
Backport-PR-URL: #32053
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 30, 2020
@mmarchini @codebytere
deps: V8: backport f7771e5b0cc4
e2c40cc 
Original commit message:

    [runtime] Recompute enumeration indices of dictionaries upon bitfield overflow

    Otherwise we'll get weird semantics when enumerating objects after many
    deletes/reinserts.

    Bug: chromium:1033771
    Change-Id: If0a459169c3794a30d9632d09e80da3cfcd4302c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1993966
    Commit-Queue: Toon Verwaest <[email protected]>
    Reviewed-by: Jakob Kummerow <[email protected]>
    Reviewed-by: Victor Gomes <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#65690}

Refs: v8/v8@f7771e5

PR-URL: #31957
Backport-PR-URL: #32053
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gengjiawen gengjiawen gengjiawen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@MylesBorins MylesBorins MylesBorins approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mmarchini @nodejs-github-bot @MylesBorins @codebytere @addaleax @cjihrig @gengjiawen